-
Notifications
You must be signed in to change notification settings - Fork 161
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: adjust inVirtual logic(#227) #262
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
是不是你antd版本太低了,最新版本没这问题了 |
@beautiful-boyyy 不是,你按照我的步骤是可以复现的,只是这个问题出现的概率较小(实际上就是你传入的:最小高度 * 数量 < 容器高度,但是 item 每一项的实际高度相加后 > 容器高度时,就会出现这个问题),最新版本的 antd 的虚拟滚动都有这个问题 |
提供一个codesandbox demo吧 |
|
貌似没权限 |
忘记开了,再试下 |
你提供的demo没有用到antd List,antd List没有暴露 height,itemHeight接口,之前的bug也在前几个版本就修复了,virtual-list这里的逻辑确实有问题,单独使用的话确实会有bug,我之前也提了一个pr,但是测试没跑过,你可能需要提供正确的测试用例。 |
src/List.tsx
Outdated
// ================================= MISC ================================= | ||
const useVirtual = !!(virtual !== false && height && itemHeight); | ||
const inVirtual = useVirtual && data && (itemHeight * data.length > height || !!scrollWidth); | ||
const containerHeight = React.useMemo(() => Object.values(heights.maps).reduce((total, curr) => total + curr, 0), [heights.id]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heights.maps 不需要 memo deps吗?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
heights.maps 引用压根不会变,不过确实加上更好点
用例能覆盖到吗? |
测试已经加上了,本地跑的没啥问题 |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #262 +/- ##
=======================================
Coverage 98.23% 98.24%
=======================================
Files 18 18
Lines 681 684 +3
Branches 160 163 +3
=======================================
+ Hits 669 672 +3
Misses 12 12 ☔ View full report in Codecov by Sentry. |
@@ -307,6 +308,8 @@ export function RawList<T>(props: ListProps<T>, ref: React.Ref<ListRef>) { | |||
const getVirtualScrollInfo = () => ({ | |||
x: isRTL ? -offsetLeft : offsetLeft, | |||
y: offsetTop, | |||
maxScrollWidth: !!scrollWidth ? scrollWidth - size.width : 0, | |||
maxScrollHeight: scrollHeight > height ? maxScrollHeight : 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个的计算我怎么有点没看懂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maxScrollWidth:scrollWidth 可能不传,所以这里加了个判断。
maxScrollHeight:这里 scrollHeight 是取的 fillerInnerRef.current?.offsetHeight(感觉这里应该取容器的 scrollHeight),会出现 scrollHeight 小于容器 height 的情况,所以要做个判断
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这俩属性和bugfix无关,我发PR去掉先。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yoyo837 麻烦问下有发版计划吗,有个项目用的这个包,想修复这个问题 |
已知会Owner. |
@@ -307,6 +308,8 @@ export function RawList<T>(props: ListProps<T>, ref: React.Ref<ListRef>) { | |||
const getVirtualScrollInfo = () => ({ | |||
x: isRTL ? -offsetLeft : offsetLeft, | |||
y: offsetTop, | |||
maxScrollWidth: !!scrollWidth ? scrollWidth - size.width : 0, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
尽量避免给测试用的打孔,这个去掉吧。测试换种方式测
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://github.com/react-component/virtual-list/releases/tag/v3.11.5 |
ok |
复现步骤:
close #243
close ant-design/ant-design#45935 again